Reject invalid value arithmetic in pallas-validate#784
Conversation
Validate ledger value arithmetic with checked conversions so invalid lovelace overflow and negative mint/burn results return validation errors instead of wrapping into large u64 values. Related: txpipe#783 Tested: RUSTC_WRAPPER= cargo test -p pallas-validate Tested: cargo-fuzz fixed seeds for value_arithmetic_invariants and minted_value_invariants
📝 WalkthroughWalkthroughLovelace addition in ChangesOverflow-safe value arithmetic
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pallas-validate/src/utils.rs (1)
175-183:⚠️ Potential issue | 🔴 CriticalFix unsafe
.unwrap()inconway_coerce_to_coin.Line 419 uses
.unwrap()onPositiveCoin::try_from(*amount), causing panics on invalid multiasset quantities instead of returning aValidationError. This contradicts the safetry_fromconversion pattern used elsewhere. Replace the.unwrap()with proper error propagation:aa.push((asset_name.clone(), PositiveCoin::try_from(*amount).map_err(|_| err.clone())?));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pallas-validate/src/utils.rs` around lines 175 - 183, In the conway_coerce_to_coin function at line 419, replace the unsafe .unwrap() call on PositiveCoin::try_from(*amount) with proper error propagation. Instead of unwrapping which causes panics, use .map_err() to convert the error to a ValidationError and propagate it with the ? operator, similar to the pattern used elsewhere in the codebase. This ensures invalid multiasset quantities are properly handled as validation errors rather than causing panics.
🧹 Nitpick comments (1)
pallas-validate/src/utils.rs (1)
369-379: 💤 Low valueConsider validated conversion from
Cointoi64.The cast
*amount as i64(line 374) could silently wrap to negative if an asset quantity exceedsi64::MAX. While practically unlikely in current Cardano usage, this could cause valid large values to be rejected after round-tripping throughadd_multiasset_values → coerce_to_coin.This is a pre-existing concern and may be out of scope for this PR.
♻️ Suggested improvement
- aa.push((asset_name.clone(), *amount as i64)); + aa.push((asset_name.clone(), i64::try_from(*amount).map_err(|_| /* overflow error */)?));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pallas-validate/src/utils.rs` around lines 369 - 379, The coerce_to_i64 function performs an unchecked cast of `*amount as i64` which can silently overflow to negative values if the asset quantity exceeds i64::MAX. Replace the direct cast with a validated conversion approach using i64::try_from() to safely handle the conversion from Coin to i64, and implement appropriate error handling for cases where the value cannot fit in i64 (either by returning a Result type or handling the overflow error case explicitly).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pallas-validate/src/utils.rs`:
- Around line 175-183: In the conway_coerce_to_coin function at line 419,
replace the unsafe .unwrap() call on PositiveCoin::try_from(*amount) with proper
error propagation. Instead of unwrapping which causes panics, use .map_err() to
convert the error to a ValidationError and propagate it with the ? operator,
similar to the pattern used elsewhere in the codebase. This ensures invalid
multiasset quantities are properly handled as validation errors rather than
causing panics.
---
Nitpick comments:
In `@pallas-validate/src/utils.rs`:
- Around line 369-379: The coerce_to_i64 function performs an unchecked cast of
`*amount as i64` which can silently overflow to negative values if the asset
quantity exceeds i64::MAX. Replace the direct cast with a validated conversion
approach using i64::try_from() to safely handle the conversion from Coin to i64,
and implement appropriate error handling for cases where the value cannot fit in
i64 (either by returning a Result type or handling the overflow error case
explicitly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdddb354-d085-47c1-99a0-27953df2971c
📒 Files selected for processing (2)
pallas-validate/src/utils.rspallas-validate/tests/utils.rs
2 participants
Summary
add_valuesandconway_add_valuesi64tou64Closes #783
Validation
RUSTC_WRAPPER= cargo test -p pallas-validateSummary by CodeRabbit